Skip to content

GH-45523: [R] Implement Utf8View type bindings - #49712

Draft
thisisnic wants to merge 24 commits into
apache:mainfrom
thisisnic:GH-45523-ipc-polars
Draft

GH-45523: [R] Implement Utf8View type bindings#49712
thisisnic wants to merge 24 commits into
apache:mainfrom
thisisnic:GH-45523-ipc-polars

Conversation

@thisisnic

@thisisnic thisisnic commented Apr 11, 2026

Copy link
Copy Markdown
Member

Rationale for this change

No bindings for Utf8View type in the R package

What changes are included in this PR?

Implement bindings

Are these changes tested?

Yep

Are there any user-facing changes?

Yep, adding functionality.

AI Usage

Heavily used Codex/Claude here. I'm not confident of every line of code. I read things over, and iterated on it making sure that tests pass and nothing seemed wildly incorrect.

@thisisnic

Copy link
Copy Markdown
Member Author

We should rebase once #49710 is merged as changes from that PR are in this branch as they were needed to make it work.

Comment on lines +829 to +830
return this->value_builder_->Append(
std::string_view(view_.bytes, static_cast<size_t>(view_.size)));

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code passed (const char*, int32_t) which matches StringBuilder::Append but not StringViewBuilder::Append (which takes int64_t). Switching to std::string_view works for both builder types.

Comment thread r/src/r_to_arrow.cpp
@thisisnic

Copy link
Copy Markdown
Member Author

A lot of .Rd updates as I used a newer roxygen2 version.

Copilot AI review requested due to automatic review settings May 28, 2026 09:37
@thisisnic
thisisnic force-pushed the GH-45523-ipc-polars branch from de59365 to cf9ad10 Compare May 28, 2026 09:37
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds R bindings and conversion support for Arrow utf8_view / string_view, addressing IPC/table conversion failures for data containing StringView columns.

Changes:

  • Adds string_view() R data type binding, export registration, and type tests.
  • Implements R↔Arrow conversion paths for StringView arrays and dictionary values.
  • Updates generated documentation and related converter support in C++/Python.

Reviewed changes

Copilot reviewed 13 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
r/R/type.R Adds StringView R6 type, constructor, and canonical type aliases.
r/R/arrowExports.R Registers R wrapper for StringView__initialize.
r/NAMESPACE Exports string_view.
r/src/datatype.cpp Maps Arrow STRING_VIEW to R StringView and initializes utf8_view.
r/src/r_to_arrow.cpp Adds R-to-Arrow StringView conversion and dictionary StringView value handling.
r/src/array_to_vector.cpp Adds Arrow-to-R StringView and wider dictionary index conversion support.
r/src/arrowExports.cpp Registers native StringView initialization entry point.
cpp/src/arrow/util/converter.h Enables dictionary converters for StringViewType.
python/pyarrow/src/arrow/python/python_to_arrow.cc Adjusts Python dictionary StringView append call.
r/tests/testthat/test-Array.R Adds StringView array round-trip tests.
r/tests/testthat/test-Table.R Adds table/dictionary StringView and wider index tests.
r/tests/testthat/test-data-type.R Adds StringView data type and code round-trip tests.
r/DESCRIPTION Updates roxygen metadata.
r/man/data-type.Rd Adds string_view() documentation entry.
r/man/acero.Rd Regenerated Acero documentation.
r/man/arrow-package.Rd Regenerated package author documentation.
r/man/csv_convert_options.Rd Regenerated CSV conversion docs.
r/man/csv_read_options.Rd Regenerated CSV read docs.
r/man/CsvReadOptions.Rd Regenerated CSV read options docs.
r/man/enums.Rd Regenerated enum documentation.
r/man/JsonFileFormat.Rd Regenerated JSON file format docs.
r/man/reexports.Rd Regenerated reexports docs.
r/man/vctrs_extension_array.Rd Regenerated vctrs extension docs.
Files not reviewed (10)
  • r/man/CsvReadOptions.Rd: Language not supported
  • r/man/JsonFileFormat.Rd: Language not supported
  • r/man/acero.Rd: Language not supported
  • r/man/arrow-package.Rd: Language not supported
  • r/man/csv_convert_options.Rd: Language not supported
  • r/man/csv_read_options.Rd: Language not supported
  • r/man/data-type.Rd: Language not supported
  • r/man/enums.Rd: Language not supported
  • r/man/reexports.Rd: Language not supported
  • r/man/vctrs_extension_array.Rd: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread r/src/r_to_arrow.cpp Outdated
Comment thread r/man/acero.Rd Outdated
Copilot AI review requested due to automatic review settings May 28, 2026 12:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 23 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • r/man/CsvReadOptions.Rd: Language not supported
  • r/man/JsonFileFormat.Rd: Language not supported
  • r/man/acero.Rd: Language not supported
  • r/man/arrow-package.Rd: Language not supported
  • r/man/csv_convert_options.Rd: Language not supported
  • r/man/csv_read_options.Rd: Language not supported
  • r/man/data-type.Rd: Language not supported
  • r/man/enums.Rd: Language not supported
  • r/man/reexports.Rd: Language not supported
  • r/man/vctrs_extension_array.Rd: Language not supported

Comment thread r/src/r_to_arrow.cpp Outdated
@thisisnic
thisisnic force-pushed the GH-45523-ipc-polars branch from a74c9c1 to 1842e7e Compare June 10, 2026 15:10
Copilot AI review requested due to automatic review settings June 10, 2026 15:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • r/man/data-type.Rd: Language not supported

@thisisnic

Copy link
Copy Markdown
Member Author

Current CI failure is unrelated; #50219

@thisisnic
thisisnic marked this pull request as ready for review June 18, 2026 15:01
@thisisnic
thisisnic requested a review from jonkeane as a code owner June 18, 2026 15:01
Copilot AI review requested due to automatic review settings June 18, 2026 15:01
thisisnic and others added 14 commits July 28, 2026 19:44
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Antoine Pitrou <pitrou@free.fr>
@thisisnic
thisisnic force-pushed the GH-45523-ipc-polars branch from dabaca9 to 39c4c92 Compare July 28, 2026 23:47
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 28, 2026
thisisnic and others added 2 commits July 28, 2026 20:08
…nary

The offset-buffer fast path in Converter_String::Ingest_some_nulls()
hardcoded GetValues<int32_t>, which is incorrect for LargeStringArray
(int64_t offsets). Use StringArrayType::offset_type so the correct
width is resolved at compile time.

Also switch the view-type guard from a runtime is_binary_view_like()
check to if constexpr, since StringViewArray has no offset_type member
and the template would fail to instantiate otherwise.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 29, 2026
Exercise out-of-line storage (>12 bytes) in the dictionary string_view
test and add NULL + mixed lengths to the binary_view table test.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 29, 2026
Comment thread r/src/r_to_arrow.cpp
}
}
} else {
RETURN_NOT_OK(this->primitive_builder_->Reserve(s.size()));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be Reserve(size - offset) just like above, no? Which means it can also be moved out of the if constexpr.

Comment thread r/src/r_to_arrow.cpp
Comment on lines +891 to +893
p_strings += offset;
for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
SEXP si = *p_strings;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be less confusing and error-prone as (IMHO):

Suggested change
p_strings += offset;
for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
SEXP si = *p_strings;
for (R_xlen_t i = offset; i < size; i++) {
SEXP si = p_strings[i];

Comment thread r/src/r_to_arrow.cpp
}
RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));
if constexpr (is_string_view_type<T>::value) {
RETURN_NOT_OK(this->primitive_builder_->Reserve(size - offset));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure it's size - offset rather than size? What is the contract for this? cc @paleolimbot @romainfrancois

Comment thread r/src/r_to_arrow.cpp
Comment on lines +905 to +907
int64_t total_length = 0;
for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
SEXP si = *p_strings;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here:

Suggested change
int64_t total_length = 0;
for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
SEXP si = *p_strings;
int64_t total_length = 0;
for (R_xlen_t i = offset; i < size; i++) {
SEXP si = p_strings[i];

Comment thread r/src/r_to_arrow.cpp
}
RETURN_NOT_OK(this->primitive_builder_->ReserveData(total_length));

p_strings = reinterpret_cast<const SEXP*>(DATAPTR_RO(s));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this if p_strings isn't mutated above.

Comment thread r/src/r_to_arrow.cpp
Comment on lines +913 to +914
for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
SEXP si = *p_strings;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (R_xlen_t i = offset; i < size; i++, ++p_strings) {
SEXP si = *p_strings;
for (R_xlen_t i = offset; i < size; i++) {
SEXP si = p_strings[i];

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants